Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate with strong_parameters #260

Merged
merged 3 commits into from Mar 25, 2013

Conversation

taavo
Copy link
Contributor

@taavo taavo commented Feb 27, 2013

See also #236

We now check for a method permitted_params in build_resource_params, and use it instead of params if present. We're expecting permitted_params to be implemented using strong_parameters' permit method, like this:

def permitted_params
  params.permit(:widget => [:permitted_field, :other_permitted_field]) 
end

Note that this doesn't work if you use strong_parameters' require method, because whereas the above example returns a cleaned copy of params, params.require(:widget).permit(:permitted_field, :other_permitted_field) returns a cleaned copy of params[:widget].

@josemarluedke
Copy link

👍

joelmoss added a commit that referenced this pull request Mar 25, 2013
Integrate with strong_parameters
@joelmoss joelmoss merged commit 995e282 into activeadmin:master Mar 25, 2013
@brendon
Copy link

brendon commented Mar 26, 2013

So weird how something like this is merged just when I needed it! :)

It might pay to add in the README that the permitted_params method should be protected and not private as otherwise it won't be found. The strong_parameters gem recommends a pattern whereby the allowed parameters are built in a private method.

Alternatively the check for the method could include the private space?

@thedanbob
Copy link

Actually, it doesn't seem to work even when permitted_params is only protected. It isn't being called by IR in my app unless I move it to public.

@brendon
Copy link

brendon commented Apr 12, 2013

You're correct @danbob. I just checked my code and my permitted_params is in the public space too. I must have figured that out, changed it, and forgot about my comment here.

Wait, actually, I've noticed that I have it in the public space in my Active Admin controller block, but in my main app I was able to put it in the protected space and it works fine. You don't happen to be using Active Admin?

@thedanbob
Copy link

Nope, just IR and CanCan. I second the suggestion to allow it to live in private or at least protected, so I can keep all my non-action controller methods separate.

@brendon
Copy link

brendon commented Apr 12, 2013

That's fairly strange. But yes, it seems more logical to allow for the recommended SP technique.

@VorontsovIE
Copy link
Contributor

Are there any reasons to use strong parameters without #require? May be this behavior should be fixed while not so much people use inherited_resources with rails 4?
I think it can be done simultaneously with sticking to standard rails 4 #{resource_name}_params method naming because it reduces number of error and simplified integration with such libraries as cancan and others. We can generate to methods: #permitted_params which works in exactly the same way it works now and #{resource_name}_params which overrides #permitted_params and allows params#require to be used.

@taavo
Copy link
Contributor Author

taavo commented May 19, 2013

Without #require, strong parameters still prohibits fields other than those marked by #permit, which means someone can update User#email without being able to update User#is_admin, which is most of the point of strong_parameters.

I still believe that #require should throw an error if the indicated parameter isn't present, but not drill down, but that's an issue for the strong_parameters team. It looks like this came up already, and someone may have come up with a plausible solution, but it's since been reverted.

@taavo taavo deleted the strong_parameters branch May 19, 2013 19:31
@VorontsovIE
Copy link
Contributor

@taavo I only say that it's simplier to allow one use both methods and use strong parameters exactly the same way they do it usually. When you refactor code to use inherited_resources - it's too simple to forget changing allowed parameters to use permitted_resource instead of, say user_params and remove #permit call in preference of deeper nesting which was working before. I just can't understand which profits this way of handling gives us comparing to default rails 4 way of handling params. The only benefit I can see - is that this method named the same for all controllers, but we can do both methods: #user_params (defined by user) and #permitted_params (defined and used in the gem) which will just delegate to #user_params if this method exists or fallback to params[#{resource_name}] (or probably raise an exception) if not.
My first concern here is compatibility of code. Because user won't need to change anything in rails4 scaffold code and in old code using inherited_resources too (permitted_params is just already redefined by the user and thus not trying to fallback to user_params). Also cancan and other libraries can rely on rails4 naming when trying to work with strong parameters.
I'll send a pull request soon to explain my idea with code more explicitly.

@VorontsovIE
Copy link
Contributor

@taavo Sent a pull request: #286

@vitaly
Copy link
Contributor

vitaly commented Jun 17, 2013

@joelmoss @taavo How is this better then just overriding resource_params in your controller? it doesn't do anything useful when we use strong_parameters as we don't need roles in this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants